Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redunce dependency list to be package link and arrow for walking up or down #423

Closed
wants to merge 1 commit into from

Conversation

tfoote
Copy link
Member

@tfoote tfoote commented Sep 15, 2024

This fixes #388

It fixes the broken links by removing them as the extra column's info has been reduced. And while here make it symmetric so you navigate packages both up and down the dependency list.

…r down

This fixes #388 

It fixes the broken links by removing them as the extra column's info has been reduced. And while here make it symmetric so you navigate packages both up and down the dependency list.
@tfoote tfoote mentioned this pull request Sep 15, 2024
Copy link

@rkent rkent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like this, the symmetry is nice. Also, in many cases the arrows under "Dependant Packages" did not show at all, blocked by the repo id. Now they do.

Some small suggested changes:

  1. The section that you changed is under {% if n_dependants > 0 %} You also need to delete the <td> in the else clause:
-                        <td class="text-center">
-                          <span class="label label-danger">0</span>
-                        </td>
  1. With the removal of the repo link, the right hand side no longer needs a lot of space, but the columns are divided <div class="col-sm-5"> and <div class="col-sm-7">. Make both of these <div class="col-sm-6"> which will reinforce the symmetry.

One additional optional note. I don't really like the graying of the arrows (class=inactive) when a target has zero dependencies, graying usually means disabled so it is confusing to me here to use it differently. But it pre-exists, so I'd like to see it at least consistent. The left arrows track both system dependencies as well as packages, while the right only packages (that is, there are no system dependencies that depend on ROS packages). Previously it was rare to actually see the gray right arrows because of the above-mentioned blocking, now they are everywhere. If we are going to leave this, I would change it to match only ROS package dependencies so that both sides are consistent, to mean "This is the bottom (or top) of a tree of ROS dependencies". I don't really care about system dependencies for that. So I would change:

{% assign n_2nd_order_pdeps = p[1].snapshots[distro].data.pkg_deps | size %}
{% assign n_2nd_order_sdeps = p[1].snapshots[distro].data.system_deps | size %}
{% assign n_2nd_order_deps = n_2nd_order_pdeps | plus: n_2nd_order_sdeps %}

to:

{% assign n_2nd_order_deps = p[1].snapshots[distro].data.pkg_deps | size %}

@rkent
Copy link

rkent commented Sep 16, 2024

BTW I implemented my suggested changes here and they are merged into my development build at index.rosdabbler.com. I switched the gray to purple so it does not look like disabled on the arrows.

@tfoote
Copy link
Member Author

tfoote commented Sep 25, 2024

Thanks for the feedback. If you'd like to break out your changes and replace this PR with the more complete changes you proposoed I'd be happy to do that instead.

@rkent
Copy link

rkent commented Sep 25, 2024

"If you'd like to break out your changes and replace this PR with the more complete changes you proposed I'd be happy to do that instead."

I've pretty much already done that, and it is running in https://devindex.rosdabbler.com See https://github.com/rkent/rosindex/tree/tfoote-rkent-simplify-dependency-table I added one commit on top of your PR. I'll PR it, then you can decide what to do with it.

@tfoote
Copy link
Member Author

tfoote commented Sep 25, 2024

Replaced by #427

@tfoote tfoote closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken Links to Instance Lists in Dependency table
2 participants